-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shareable value rewrite #3722
Merged
Merged
Shareable value rewrite #3722
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kmagiera
added a commit
that referenced
this pull request
Dec 15, 2022
## Summary This PR fixes crashes related to concurrent writes to a non-thread-safe frameCallbacks vector. The primary role of frameCallbacks vector was to facilitate requestAnimationFrame calls and hence it has not been designed to allow for other than the main thread to append their callbacks. However, after recent rewrite #3722 we introduced a new methods "scheduleOnUI" that was originally meant to interface with a thread-safe scheduler API but was later updated (see c8a77da) to use frameCallbacks in order for errors to be handled using the same code-path the rAF uses. That change introduces a bug in which we'd access and modify that vector from different threads which is undesirable. This PR reverts that change and since #3846 provides a better way of handling JS-errors there is no longer need for scheduleOnUI callbacks to go throught the requestAnimationFrame codepath. ## Test plan The easiest way to reproduce the crash we could find was by using BokehExample.tsx on Android. This would normally result in a crash after a couple of seconds of running. With the increased number of circles in that example (e.g. 400) the crash would be almost immediate. This change was tested on that example with 400 circles and the crash would not appear even after some time after launch (few minutes). Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com> Co-authored-by: Krzysztof Piaskowy <krzysztof.piaskowy@swmansion.com> Co-authored-by: Juliusz Wajgelt <49338439+jwajgelt@users.noreply.github.com>
piaskowyk
pushed a commit
that referenced
this pull request
Dec 16, 2022
## Summary This PR removes a chunk of code that was supposed to be executed only if a worklet has some optimization applied in Babel plugin but since #3722 completely removes these optimizations, this chunk of code is no longer necessary. Co-authored-by: @kmagiera @piaskowyk ## Test plan Check if BokehExample works correctly.
3 tasks
This was referenced Feb 23, 2023
kmagiera
added a commit
that referenced
this pull request
Feb 24, 2023
## Summary Before this change we'd only call `performOperations` for a subset of filtered events (initially introduced in #312). The event's that weren't considered "direct" would require additional animation frame for having their updates flushed. This change makes it so that we call `performOperations` for all types of events therefore matching the behavior [on Android](https://github.com/software-mansion/react-native-reanimated/blob/99b8b3ed56e36ca615cce7164ccaf04d154571b1/android/src/main/java/com/swmansion/reanimated/NodesManager.java#L283) A consequence of this change was that for some event types, the updates reanimated was performing weren't getting flushed onto screen. Namely, we noticed this problem in Pager example where a view pager component with some custom set of events is used and event handlers update shared values. Even though such event would trigger shared value update, and these updates would trigger the style to recalculate and we'd even call updateProps method to apply the updated props, we'd still see no result as in that example the changes require layout run. Without `performOperation` call the layout would not be executed unless react would rerender or other time-based animation would run. The problem became apparent after #3970 where we changed the place where updates are performed from requestAnimationFrame to setImmediate. Before this change, since we were running the updates in "animation frame" the `performOperation` method was being run by the frame scheduler. We, however were getting these updates delayed by one frame because of that. This issue also wasn't noticed prior to shareable rewrite from #3722 because before, we were always starting frame updater for every single update happening to shared value even if it was due to an event. As a result, we were getting the stuff updated on screen but again, with a delay of one frame. ## Test plan Run pager example on iOS.
kmagiera
added a commit
that referenced
this pull request
Feb 24, 2023
… a given frame (#4099) ## Summary This PR fixes a perf regression introduced in #3722. The issue was that we'd schedule a native request animation frame callback several times in a single frame. As a result we'd run mapper updates more than once per frame which resulted in UI thread skipping frames. The bug would surface in examples where we had both timing (active) and gesture driven animations. In such scenario we'd enqueue native requestAnimationFrame callback twice – first time for the running timing animation, and the second time as a result of handling the touch event. These two callbacks would then run as long as there are update happening and would incur additional cost to all updates happening on the UI thread. Moreover, for each new gesture started afterwards we'd add a new call to requestAnimationFrame and yet another time the updates would be executed withing a single frame. After several times of launching new stream of events, we'd end up with a huge number of repeated work being done in a single frame which was causing significant frame drops. This PR addresses this problem by preventing requestAnimationFrame callbacks to be flushed more than once in a single frame. We do this by remembering if the previous flush was caused by the native rAF callback or was it forced by other activity (i.e. event handling). Then we only allow native rAF callback to perform a flush if it doesn't immediately follow a non-native flush. This way we don't perform additional work in a frame when the updates are already triggered by the event. ## Test plan Below is a link to a minimum repro of this issue. The app runs an infinite animation and renders a draggable circle. After running this app for a while and pulling the circle several times the performance starts to drop. https://gist.github.com/kmagiera/b2df85f9512951f5e6ceee7bc569f5f1
kmagiera
added a commit
that referenced
this pull request
Feb 26, 2023
## Summary This PR fixes issue with UI not updating when style changes are connected to events on iOS Fabric. This problem has been introduced in #3970 where we migrated from using `rAF` and started using `setImmediate` instead. As a result we had to manually flush set-immediates queue in certain conditions. Specifically this was already happening after animations frame was run (i.e. in `requestAnimationFrame` callback) but should also be done directly after handling events much like we already do on Android [here](https://github.com/software-mansion/react-native-reanimated/blob/99b8b3ed56e36ca615cce7164ccaf04d154571b1/android/src/main/java/com/swmansion/reanimated/NodesManager.java#L283) and on iOS-paper [here](https://github.com/software-mansion/react-native-reanimated/blob/d9a55c556fc32fcb5db59acc92cbedd6452af9dc/ios/REANodesManager.mm#L334). The reason this stopped working on iOS and not on Android was that Android-fabric implementation still uses old architecture flow for handling events while iOS uses new C++ based `react::EventListener` API (that apparently wasn't working on Android last time we checked). This PR adds flushing operations queue in that new C++ based flow. Note that this problem didn't occur before Shareable Rewrite (#3722) because before all mapper updates would result in us scheduling new animation frames. So the updates were happening eventually, were just delayed by one frame. ## Test plan Run Fabric example on iOS, test Article progress example.
3 tasks
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
## Description This PR contains a new implementation of the "shareable value" concept – one of the lowest level key concepts behind shared values/worklets etc. There were many reasons for attempting this rewrite, but the main objective was to address memory-related issues rooted in that core part and difficult to fix due to an inherent complexity of that bit of the codebase. Below, I list a couple of reasons providing some more details description: 1. The aforementioned memory issues can be best noted on software-mansion#2824 – the root cause of that and similar issue lies in the way we reference objects between the two runtimes. Due to the fact we use JSI's host objects and direct references it turns out that a lot of objects can outlast garbage collection because, for example they are referenced on the secondary runtime where garbage collection didn't run yet. Moreover, JSI objects have their own, simplified version of GC that has to run as well. As a result, for some objects to be cleaned up properly, we needed to run GC on the main JS runtime, then on the UI runtime and once again on the main JS runtime. 2. Secondary objective for the rewrite was to address the maintainability of the codebase part being rewritten. After investigating the issue mentioned above we concluded that it is too difficult and risky to try to fix it given the architectural choices we made in the past. 3. As a side-effect of the things being rewritten we expect the library to perform better. When tracking performance issues in the past, we detected that a lot of issues are due to an excessive communication over JSI. JSI is a lot quicker compared to calling things over "the bridge", however it still incurs some performance penalty that is significant enough on lower level devices even at a level of couple hundred calls. This issue has became apparent to us at some point, but the existing architecture wouldn't allow us to address it in a easy way. As an example we can bring up the issue with worklet functions that, before this rewrite, were represented as JSI's HostFunctions. As a result, calling worklet from on the UI thread would require a JSI roundtrip, and that has became problematic as we expanded the codebase and had one more logic extracted out into smaller worklets. This rewrite has been architected to optimize the number of cross-JSI calls. 4. The new core makes it possible to implement some new and often requested functionalities. Specifically, the ability to make complex shareable objects, that is, to be able to append to a shared array or add/remove entries from a shared map as opposed to always having to copy the whole new object with the modifications applied. Adding this also helps to improve some bits internally, specifically the case of shared styles, where we have a single animated style object assigned to multiple views. Before, we'd use an immutable array that'd represent the list of so-called view descriptors. With this rewrite we can add and remove entries without a need to copy the whole descriptors array as we mount new views. The description of this PR is also a work in progress and will update sections of this description as the work progresses. ## Changes This PR changes a lot of things under the hood. The main change lies in a way we create and reference shareable values. As part of this change the whole "notion" of shareable values is removed from the core and implemented on top of a different abstraction. We now allow for "shareable" data to provide an initializer function which is called on the UI runtime in order to instantiate data that then will be accessed by worklets. In addition, we avoid keeping JSI object cache on c++ side, this prevents issues with them being referenced for longer than necessary and hence was a root cause of memory related issues (leaks -- but these weren't actual leaks as triggering GC several times would result in a complete cleanup).
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
## Summary This PR fixes crash on Fabric when using synchronous measure/scrollTo etc – this error was a regression introduced in software-mansion#3722. The issue was that did not transform shadow node wrapper ref into shareable ref. Shareable ref was necessary for the UI side to recognize and extract the shadow node wrapper that was needed for the sync calls to execute. ## Test plan Run FabricExample, use "Measure on UI" on the "Measure Example" screen and "ScrollTo on UI" button on the "ScrollTo Example" screen.
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
…oftware-mansion#3821) ## Summary After software-mansion#3722 the error message presented after attempting to synchronously call React runtime function from the UI runtime has changed and became less descriptive than before. This PR adds a way for us to throw a more descriptive error in such scenario. ## Test plan Use the following code: ``` function rnMethod() { console.log('from RN'); } function uiMethod() { 'worklet'; rnMethod(); // this should throw } runOnUI(uiMethod)(); ``` Before this change you'd get a generic "Object is not a function" thrown at the line where we call `rnMethod`, now the error message says "Tried to synchronously call a non-worklet function" and presents possible solution (see the diff for the whole error message) Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com>
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
## Summary This PR enables weak object implementation for shareables when using V8 runtime. As @Kudo mentioned in software-mansion#3722 (comment), react-native-v8 exposes weak objects API via JSI (see [here](https://github.com/Kudo/react-native-v8/blob/96a1a2c8a35349967a3ad7219106a3475b85433a/src/v8runtime/V8Runtime.cpp#L982-L997)). ## Test plan Run Example app with react-native-v8 and check if everything works.
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
…-mansion#3859) ## Summary This PR fixes crashes related to concurrent writes to a non-thread-safe frameCallbacks vector. The primary role of frameCallbacks vector was to facilitate requestAnimationFrame calls and hence it has not been designed to allow for other than the main thread to append their callbacks. However, after recent rewrite software-mansion#3722 we introduced a new methods "scheduleOnUI" that was originally meant to interface with a thread-safe scheduler API but was later updated (see c8a77da) to use frameCallbacks in order for errors to be handled using the same code-path the rAF uses. That change introduces a bug in which we'd access and modify that vector from different threads which is undesirable. This PR reverts that change and since software-mansion#3846 provides a better way of handling JS-errors there is no longer need for scheduleOnUI callbacks to go throught the requestAnimationFrame codepath. ## Test plan The easiest way to reproduce the crash we could find was by using BokehExample.tsx on Android. This would normally result in a crash after a couple of seconds of running. With the increased number of circles in that example (e.g. 400) the crash would be almost immediate. This change was tested on that example with 400 circles and the crash would not appear even after some time after launch (few minutes). Co-authored-by: Tomek Zawadzki <tomasz.zawadzki@swmansion.com> Co-authored-by: Krzysztof Piaskowy <krzysztof.piaskowy@swmansion.com> Co-authored-by: Juliusz Wajgelt <49338439+jwajgelt@users.noreply.github.com>
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
## Summary This PR removes a chunk of code that was supposed to be executed only if a worklet has some optimization applied in Babel plugin but since software-mansion#3722 completely removes these optimizations, this chunk of code is no longer necessary. Co-authored-by: @kmagiera @piaskowyk ## Test plan Check if BokehExample works correctly.
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
## Summary Before this change we'd only call `performOperations` for a subset of filtered events (initially introduced in software-mansion#312). The event's that weren't considered "direct" would require additional animation frame for having their updates flushed. This change makes it so that we call `performOperations` for all types of events therefore matching the behavior [on Android](https://github.com/software-mansion/react-native-reanimated/blob/99b8b3ed56e36ca615cce7164ccaf04d154571b1/android/src/main/java/com/swmansion/reanimated/NodesManager.java#L283) A consequence of this change was that for some event types, the updates reanimated was performing weren't getting flushed onto screen. Namely, we noticed this problem in Pager example where a view pager component with some custom set of events is used and event handlers update shared values. Even though such event would trigger shared value update, and these updates would trigger the style to recalculate and we'd even call updateProps method to apply the updated props, we'd still see no result as in that example the changes require layout run. Without `performOperation` call the layout would not be executed unless react would rerender or other time-based animation would run. The problem became apparent after software-mansion#3970 where we changed the place where updates are performed from requestAnimationFrame to setImmediate. Before this change, since we were running the updates in "animation frame" the `performOperation` method was being run by the frame scheduler. We, however were getting these updates delayed by one frame because of that. This issue also wasn't noticed prior to shareable rewrite from software-mansion#3722 because before, we were always starting frame updater for every single update happening to shared value even if it was due to an event. As a result, we were getting the stuff updated on screen but again, with a delay of one frame. ## Test plan Run pager example on iOS.
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
… a given frame (software-mansion#4099) ## Summary This PR fixes a perf regression introduced in software-mansion#3722. The issue was that we'd schedule a native request animation frame callback several times in a single frame. As a result we'd run mapper updates more than once per frame which resulted in UI thread skipping frames. The bug would surface in examples where we had both timing (active) and gesture driven animations. In such scenario we'd enqueue native requestAnimationFrame callback twice – first time for the running timing animation, and the second time as a result of handling the touch event. These two callbacks would then run as long as there are update happening and would incur additional cost to all updates happening on the UI thread. Moreover, for each new gesture started afterwards we'd add a new call to requestAnimationFrame and yet another time the updates would be executed withing a single frame. After several times of launching new stream of events, we'd end up with a huge number of repeated work being done in a single frame which was causing significant frame drops. This PR addresses this problem by preventing requestAnimationFrame callbacks to be flushed more than once in a single frame. We do this by remembering if the previous flush was caused by the native rAF callback or was it forced by other activity (i.e. event handling). Then we only allow native rAF callback to perform a flush if it doesn't immediately follow a non-native flush. This way we don't perform additional work in a frame when the updates are already triggered by the event. ## Test plan Below is a link to a minimum repro of this issue. The app runs an infinite animation and renders a draggable circle. After running this app for a while and pulling the circle several times the performance starts to drop. https://gist.github.com/kmagiera/b2df85f9512951f5e6ceee7bc569f5f1
fluiddot
pushed a commit
to wordpress-mobile/react-native-reanimated
that referenced
this pull request
Jun 5, 2023
## Summary This PR fixes issue with UI not updating when style changes are connected to events on iOS Fabric. This problem has been introduced in software-mansion#3970 where we migrated from using `rAF` and started using `setImmediate` instead. As a result we had to manually flush set-immediates queue in certain conditions. Specifically this was already happening after animations frame was run (i.e. in `requestAnimationFrame` callback) but should also be done directly after handling events much like we already do on Android [here](https://github.com/software-mansion/react-native-reanimated/blob/99b8b3ed56e36ca615cce7164ccaf04d154571b1/android/src/main/java/com/swmansion/reanimated/NodesManager.java#L283) and on iOS-paper [here](https://github.com/software-mansion/react-native-reanimated/blob/d9a55c556fc32fcb5db59acc92cbedd6452af9dc/ios/REANodesManager.mm#L334). The reason this stopped working on iOS and not on Android was that Android-fabric implementation still uses old architecture flow for handling events while iOS uses new C++ based `react::EventListener` API (that apparently wasn't working on Android last time we checked). This PR adds flushing operations queue in that new C++ based flow. Note that this problem didn't occur before Shareable Rewrite (software-mansion#3722) because before all mapper updates would result in us scheduling new animation frames. So the updates were happening eventually, were just delayed by one frame. ## Test plan Run Fabric example on iOS, test Article progress example.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 28, 2023
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary * Ts-prune allows us to detect unused typescript code. * Advantages: * We can remove exports of all the functions and interfaces exported for no reason. This makes importing with cmd+enter shortcut easier, since you are less likely to get wrong suggestions from vscode. * We can see all the “forgotten” areas of code, and think about reasons to have them. <!-- Explain the motivation for this PR. Include "Fixes #<number>" if applicable. --> > **Note** >I've removed the file `MapperRegistry.ts` which seems unnecessary since this PR: [#3722](#3722) ## Test plan Just CI <!-- Provide a minimal but complete code snippet that can be used to test out this change along with instructions how to run it and a description of the expected behavior. --> --------- Co-authored-by: Aleksandra Cynk <aleksandracynk@swmansion.com>
piaskowyk
pushed a commit
that referenced
this pull request
Aug 4, 2023
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary * Ts-prune allows us to detect unused typescript code. * Advantages: * We can remove exports of all the functions and interfaces exported for no reason. This makes importing with cmd+enter shortcut easier, since you are less likely to get wrong suggestions from vscode. * We can see all the “forgotten” areas of code, and think about reasons to have them. <!-- Explain the motivation for this PR. Include "Fixes #<number>" if applicable. --> > **Note** >I've removed the file `MapperRegistry.ts` which seems unnecessary since this PR: [#3722](#3722) ## Test plan Just CI <!-- Provide a minimal but complete code snippet that can be used to test out this change along with instructions how to run it and a description of the expected behavior. --> --------- Co-authored-by: Aleksandra Cynk <aleksandracynk@swmansion.com>
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR contains a new implementation of the "shareable value" concept – one of the lowest level key concepts behind shared values/worklets etc.
There were many reasons for attempting this rewrite, but the main objective was to address memory-related issues rooted in that core part and difficult to fix due to an inherent complexity of that bit of the codebase. Below, I list a couple of reasons providing some more details description:
The description of this PR is also a work in progress and will update sections of this description as the work progresses.
Changes
This PR changes a lot of things under the hood. The main change lies in a way we create and reference shareable values. As part of this change the whole "notion" of shareable values is removed from the core and implemented on top of a different abstraction. We now allow for "shareable" data to provide an initializer function which is called on the UI runtime in order to instantiate data that then will be accessed by worklets. In addition, we avoid keeping JSI object cache on c++ side, this prevents issues with them being referenced for longer than necessary and hence was a root cause of memory related issues (leaks -- but these weren't actual leaks as triggering GC several times would result in a complete cleanup).
Checklist